Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ddl: cancel add index when can not find partition and fix GetPartition function bug #10475

Merged
merged 12 commits into from
May 20, 2019

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented May 15, 2019

What problem does this PR solve?

Below sql will cause tidb panic when add index.

create table t1 (a int,b int,  primary key(a)) partition by hash(a) partitions 99;
insert into t1 set a=1;
insert into t1 set a=0;
alter table t1 add index idx(a);  # panic

What is changed and how it works?

  1. cancel add index ddl job when can not find partition
  2. Fix GetPartition function bug.
  3. Make alter table t1 add index idx(a); execute successful.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

Related changes

  • Need to cherry-pick to the release branch

@@ -313,7 +313,11 @@ func (t *partitionedTable) locateHashPartition(ctx sessionctx.Context, pi *model

// GetPartition returns a Table, which is actually a partition.
func (t *partitionedTable) GetPartition(pid int64) table.PhysicalTable {
return t.partitions[pid]
p, ok := t.partitions[pid]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eg:

type partition struct {
	tableCommon
}

type partitionTables struct {
	ps map[int]*partition
}

func (p *partition) GetPhysicalID() int {
	return 0
}

type tableCommon struct {
	tableID int64
}

type physibleTable interface {
	GetPhysicalID() int
}

func (pt *partitionTables) getPartition(id int) physibleTable {
	return pt.ps[id]
}

func (pt *partitionTables) getPartition2(id int) physibleTable {
	p, ok := pt.ps[id]
	if !ok {
		return nil
	}
	return p
}

func main() {
	ps := make(map[int]*partition)
	pt := &partitionTables{ps: ps}
	fmt.Println(pt.getPartition(0) == nil)  // false
	fmt.Println(pt.getPartition2(0) == nil) // true
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Golang traps and pitfalls...

A nil of type *partition is a kind of physibleTable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! @crazycs520

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to add some comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@codecov
Copy link

codecov bot commented May 15, 2019

Codecov Report

Merging #10475 into master will decrease coverage by 0.0119%.
The diff coverage is 42.8571%.

@@               Coverage Diff               @@
##             master     #10475       +/-   ##
===============================================
- Coverage   77.2752%   77.2633%   -0.012%     
===============================================
  Files           413        413               
  Lines         86967      86974        +7     
===============================================
- Hits          67204      67199        -5     
- Misses        14599      14608        +9     
- Partials       5164       5167        +3

@crazycs520 crazycs520 requested review from tiancaiamao and zimulala and removed request for tiancaiamao May 15, 2019 05:55
@crazycs520
Copy link
Contributor Author

@winkyao @zimulala @tiancaiamao PTAL

// update them to table's in this case.
if endHandle == 0 || physicalTableID == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why ignore endHandle here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because endHandle is 0 don't mean doesn't store them before. Maybe the stored endHandle is 0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any test to cover here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -313,7 +313,13 @@ func (t *partitionedTable) locateHashPartition(ctx sessionctx.Context, pi *model

// GetPartition returns a Table, which is actually a partition.
func (t *partitionedTable) GetPartition(pid int64) table.PhysicalTable {
return t.partitions[pid]
// Attention, can't simply use `return t.partitions[pid]` here.
// Because A nil of type *partition is a kind of `table.PhysicalTable`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Because A nil of type *partition is a kind of `table.PhysicalTable`
// Because A nil of type *partition is a kind of `table.PhysicalTable`,
// the final result will be a non-nil interface whose underlying value is nil,
// use that result will get a nil pointer panic.

@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label May 16, 2019
@tiancaiamao
Copy link
Contributor

/run-all-tests

Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zimulala zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 16, 2019
err = w.runReorgJob(t, reorgInfo, d.lease, func() error {
err = w.runReorgJob(t, reorgInfo, d.lease, func() (addIndexErr error) {
defer func() {
r := recover()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use util.WithRecover instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I need return cancelled error here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can change util.WithReocover to return value from exec func

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, go WithReocover can not return values

@crazycs520
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/sql-infra SIG: SQL Infra status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants